Add FastAPI support with an ASGI Middleware#105
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
|
Thanks @artefactop for the PR. Will take a look. |
sjs994
left a comment
There was a problem hiding this comment.
Thanks @artefactop for the change. Apart from few comments added, looks good to me.
| @mock.patch('google.cloud.sqlcommenter.sqlalchemy.executor.get_fastapi_info', return_value=fastapi_info) | ||
| def test_all_data(self, get_info): | ||
| self.assertSQL( | ||
| "SELECT 1; /*controller='c',framework='fastapi',route='/'*/", |
There was a problem hiding this comment.
Sorry to miss this one.
Although, this change looks good, wanted to give you an FYI on an existing bug in SQLAlchemy and psycopg2 sqlcommenter:
This query:
SELECT 1; /*controller='c',framework='fastapi',route='/'*/
In general a ; ends a SQL query. So, any comment added after that is not really part of that query. So, even with this change, my guess is SQL comments would not work as expected as DB will assume there are 2 queries:
- SELECT 1;
- /*controller='c',framework='fastapi',route='/'*/
I have added one small PR for psycopg2 as of now (Waiting for review) and will create another for SQLAlchemy if the approach doesn't have major concern.
|
@sjs994 should I merge this PR or you do? |
|
@artefactop I can do that. |
Can be easily extensible for other ASGI frameworks.